Skip to content

Conversation

@titouanc
Copy link
Contributor

@titouanc titouanc commented Jul 31, 2025

Add a new Network MIDI2.0 Host stack, allowing Zephyr to expose a Universal MIDI Packet endpoint over UDP, so that clients can connect and exchange MIDI data over the network. The host optionally supports authentication with a single shared secret, or with a list of user/password.

image

This initial implementation is based on the following document:
User Datagram Protocol for Universal MIDI Packets - Network MIDI 2.0 (UDP) Transport Specification - Document version 1.0

In addition to the new network protocol, other related changes are shipped in this PR:

  • Add a new library UMP Stream Responder, which allows for network clients to discover the UMP Endpoint topology
  • Adapt the existing USB-MIDI2.0 sample to use the above responder
  • Add a new Olimex MIDI shield
  • Add a code sample for the new Network MIDI 2.0 host stack
  • Minor improvements to the documentation of existing MIDI UMP definitions

(To the best of my knowledge, this would make Zephyr the first OS with mainline support for Network MIDI 2.0)

Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, I have mostly minor style related nits

@titouanc
Copy link
Contributor Author

titouanc commented Aug 5, 2025

Thanks @jukkar for your initial review 🙂

I just rebased on top of main, and pushed the following changes:

  • Use IPv6 if available
  • Document struct/enum members where missing
  • Perform authentication hash in multiple parts rather than concatenating everything in an arbitrary-sized input before performing the hash in one pass. This avoids a magic constant and possibly some out-of-bounds bugs
  • Miscellaneous code style fixes
  • Fix invalid sample.yaml
  • Remove default configuration credentials in the sample application

}

/* 1. Start hashing with the session nonce */
hash.in_buf = (uint8_t *) sess->nonce;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a side note, this is currently const-incorrect.

This can be fixed if we merge #94218


if (sess->ep->auth_type == NETMIDI2_AUTH_SHARED_SECRET) {
/* 2. Finalize hashing with the shared secret */
hash.in_buf = (uint8_t *) sess->ep->shared_auth_secret;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

net_buf_pull(buf, payload_len - NETMIDI2_DIGEST_SIZE);

/* 2. Continue hashing with the username */
hash.in_buf = (uint8_t *) user->name;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

}

/* 3. Finalize hashing with the password */
hash.in_buf = (uint8_t *) user->password;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And again same here

@@ -0,0 +1,18 @@
CONFIG_NETWORKING=y
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you probably want to enable CONFIG_TEST_RANDOM_GENERATOR somewhere here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I just see your comment now, while I just pushed a different solution.

Since only the mps2/an385 platform is causing problem, I excluded it from the tests definitions in the sample; but I don't believe this is the clean solution, only the one where the CI would pass.

Maybe we could enable CONFIG_TEST_RANDOM_GENERATOR instead, but only on boards that require it ? This looks like an unsafe option to enable unconditionally ?

Thank you for looking into that 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah not entirely sure what story is but CONFIG_TEST_RANDOM_GENERATOR seems to be what's done in all other samples under net/. With the exclude it passes CI but then it'd break if someone tries to build the sample against a specific platform with no rng -- probably not a real problem, doubt there's anything out there with ethernet but no rng but I think there's value in keeping the net sample structure uniform.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes indeed, keeping samples consistent is a very good reason to do so, so I just pushed that instead.
Thanks !

@fabiobaltieri
Copy link
Member

fabiobaltieri commented Sep 22, 2025

...also, you could probably send the shield commit in its own PR if you want to get that one off the way of this one, it'd probably get in pretty quickly, up to you.

Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of minor nits. Also please do not resolve comments yourself but let reviewer do that. It helps the review process a bit as reviewer can mark the comment resolved when done.
See https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#workflow-suggestions-that-help-reviewers for more info.

/** Sequence number of the next universal MIDI packet to receive */
uint16_t rx_ump_seq;
/** Remote address of the peer */
struct sockaddr addr;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works in zephyr because we allocate enough storage in struct sockaddr for the largest address in use but this would fail for example if done in Linux. So in theory one should use struct sockaddr_storage when storing the socket address, and struct sockaddr should only be used when passing socket information in functions.
Note that there is static inline struct sockaddr *net_sad(const struct sockaddr_storage *addr); to convert from storage to sockaddr.
Not sure how big change this would be, but might be good to change this if it is not too much work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will look into that, thanks for pointing it out !

Comment on lines 83 to 85
static inline const struct netmidi2_user *netmidi2_find_user(
const struct netmidi2_ep *ep, const char *name, size_t namelen
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation and formatting looks different than the other functions, perhaps change to

static inline const struct netmidi2_user *netmidi2_find_user(const struct netmidi2_ep *ep,
							     const char *name,
							     size_t namelen)

Note that there are other functions in this file that look a bit non-zephyr like indentation wise too.

@titouanc
Copy link
Contributor Author

Thanks for your latest review @jukkar !

I just rebased on top of the latest main, applying the following changes:

  • Properly reindent some internal functions in netmidi2.c to follow codestyle
  • Use sockaddr_storage instead of sockaddr as suggested

@jukkar
Copy link
Member

jukkar commented Oct 1, 2025

@titouanc can you rebase to latest main and force push in order to get rid of CI failure

Improve the API documentation for Universal MIDI Packets definitions:

- Add link to the reference document, and make it referenceable in doxygen
- Use BIT_MASK macro instead of hexadecimal litterals to better convey the
  length of various fields within UMP packets
- Add more cross-references where possible
- Use @remark when applicable

Signed-off-by: Titouan Christophe <[email protected]>
Add a new top-level, transport independent library to respond to UMP Stream
Discovery messages. This allows MIDI2.0 clients to discover UMP endpoints
hosted on Zephyr over the UMP protocol.

The endpoint specification can be gathered from the device tree, so that
the same information used to generate USB descriptors in usb-midi2.0
can be delivered over UMP Stream.

Signed-off-by: Titouan Christophe <[email protected]>
Since a new device-tree property has been introduced to describe if a
UMP group is limited to 31.25kb/s (like a physical MIDI DIN-5 port),
this can be used in the USB UMP group terminal block descriptors, as
specified in the USB-MIDI2.0 reference document [1], (in 5.4.2.1
Group Terminal Block Descriptor):

  Maximum Output Bandwidth Capability in 4KB/second. 0x0000 – 0xFFFF.
  Bandwidth is total for this Block, shared among all OUT Group Terminals.
      0x0000 = Unknown or Not Fixed.
      0x0001 = Rounded version of 31.25kb/s

[1] https://www.usb.org/sites/default/files/USB%20MIDI%20v2_0.pdf

Signed-off-by: Titouan Christophe <[email protected]>
Update the USB-MIDI2.0 sample to use the newly introduced
UMP Stream Responder library. This allows the host to discover the
topology and function block names (if supported by the host OS).

Signed-off-by: Titouan Christophe <[email protected]>
Add a new network protocol for MIDI2.0 over the network, using UDP sockets.
This allows Zephyr to host a UMP endpoint on the network, which can be
invited by UMP clients to exchange MIDI2.0 data.

Signed-off-by: Titouan Christophe <[email protected]>
Add a new very simple shield that provides MIDI DIN-5 IN/OUT/THROUGH,
as well as 2 leds. Other features of the shields (capacitive sensors)
are currently not reified in Zephyr.

While there is no further device on the shield itself (everything goes
through the Arduino header directly), this allows for meaningful
device-tree naming when using the shield.

Signed-off-by: Titouan Christophe <[email protected]>
Add a new sample to demonstrate usage of the newly introduced
Network MIDI 2.0 host stack.

Signed-off-by: Titouan Christophe <[email protected]>
@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 1, 2025

Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to the networking changes

Copy link
Contributor

@kartben kartben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refreshing earlier approval :)

@kartben kartben merged commit 3c50822 into zephyrproject-rtos:main Oct 8, 2025
27 checks passed
@butok
Copy link
Contributor

butok commented Oct 8, 2025

Hi @titouanc
It looks like this PR has caused CI errors for samples/subsys/usb/midi, blocking other PR: https://github.com/zephyrproject-rtos/zephyr/actions/runs/18343615375/job/52245283869?pr=97185
Could you look at it?

@titouanc
Copy link
Contributor Author

titouanc commented Oct 8, 2025

Hello @butok looking at this now.
Any idea why it hasn't been caught in this PR in the first place ?

@butok
Copy link
Contributor

butok commented Oct 8, 2025

Hello @butok looking at this now. Any idea why it hasn't been caught in this PR in the first place ?

Unfortunately, Zephyr CI is not reliable; it repeatedly does not catch errors

@titouanc titouanc deleted the add-netmidi2 branch October 14, 2025 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.